Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support vectorized interpolation with more scipy interpolators #9526

Merged
merged 3 commits into from
Sep 25, 2024
Merged

Support vectorized interpolation with more scipy interpolators #9526

merged 3 commits into from
Sep 25, 2024

Conversation

hollymandel
Copy link
Contributor

@hollymandel hollymandel commented Sep 20, 2024 •
edited
Loading

@max-sixty
Copy link
Collaborator

Something has gone kinda wrong with git here! Maybe try squashing to a single commit?

@hollymandel
Copy link
Contributor Author

Something has gone kinda wrong with git here! Maybe try squashing to a single commit?

yes, done, thanks!

@max-sixty
Copy link
Collaborator

Nice, thanks!

(I don't know this code well so will defer to others on the review, hope that's OK...)

@hollymandel
Copy link
Contributor Author

Sounds good! I actually wanted it to be a "draft PR" as I'm still double-checking it, though perhaps submitting it this way is confusing on the maintainers' end and I should rescind until it's ready.

@keewis
Copy link
Collaborator

keewis commented Sep 23, 2024 •
edited
Loading

though perhaps submitting it this way is confusing on the maintainers' end and I should rescind until it's ready

we're actually happy to have PRs developed like this, it allows us to guide people more easily in case there's any issues. This includes advising in case the direction is seriously wrong (which I don't think it is in this case, but I also don't know this area too well)

Just tell us when you think this is ready or if you need any help.

@hollymandel hollymandel force-pushed the interp_vectorize branch 2 times, most recently from 9ec3e07 to 9c33edf Compare September 23, 2024 19:44
@hollymandel hollymandel marked this pull request as ready for review September 23, 2024 19:48
dcherian
dcherian reviewed Sep 24, 2024
View reviewed changes
xarray/core/missing.py Outdated Show resolved Hide resolved
dcherian
dcherian reviewed Sep 24, 2024
View reviewed changes
xarray/core/missing.py
@@ -614,9 +623,6 @@ def interp(var, indexes_coords, method: InterpOptions, **kwargs):
if not indexes_coords:
return var.copy()

# default behavior
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice cleanup!

dcherian
dcherian reviewed Sep 24, 2024
View reviewed changes
xarray/tests/test_interp.py Outdated
Comment on lines 146 to 147
if not has_scipy:
pytest.skip("scipy is not installed.")
Copy link
Contributor

@dcherian dcherian Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not has_scipy:
pytest.skip("scipy is not installed.")

driveby cleanup. The requires_scipydecorator will skip the test if scipy isn't available. Import it from xarray.tests if it's not present.

dcherian
dcherian reviewed Sep 24, 2024
View reviewed changes
xarray/core/missing.py Outdated
@@ -755,8 +761,9 @@ def interp_func(var, x, new_x, method: InterpOptions, kwargs):

def _interp1d(var, x, new_x, func, kwargs):
# x, new_x are tuples of size 1.
x, new_x = x[0], new_x[0]
rslt = func(x, var, assume_sorted=True, **kwargs)(np.ravel(new_x))
x, new_x = x[0].data, new_x[0].data
Copy link
Contributor

@dcherian dcherian Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The addition of .data is significant. Where did the types change? We seem to have moved from np.ndarray to Variable?
  2. Is there a reason you got rid of assume_sorted=True?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This doesn't seem to have been necessary, changing back

  2. The interpolators I have added (Scipy.Interpolate.Akima1DInterpolator, etc...) require sorted axes but don't take assume_sorted as an argument. Only Scipy.Interpolate.Interp1d takes this argument. Since we are always calling it with True, I'm just letting the default for ScipyInterpolator init impose this.

There is maybe a philosophical conflict here and in previous comments about kwarg handling for the different interpolators. The preexisting script (missing.py) has special classes (NumpyInterpolator, ScipyInterpolator...) which basically just do this handling. I didn't add any new classes for the new interpolators and rather tried to do everything with kwargs determined by _get_interpolator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re (1): If you're comfortable with typing, some type hints would go a long way towards improving this module.

Re (2): sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(1) Yeah can do! Prefer a separate commit though (?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we squash merge so it doesn't matter :) unless you meant separate PR, in which case that would be fine too.

@dcherian
Copy link
Contributor

dcherian commented Sep 24, 2024 •
edited
Loading

Nice progress! I pushed a commit that should handle the min scipy version failure with "makima".

Please add a note to whats-new.rst also.

I wanted to add a deprecation warning

Yes, we error on unexpected warnings. To add this, catch the warning using

with pytest.warns(DeprecationWarning):
    ...

though perhaps submitting it this way is confusing on the maintainers' end and I should rescind until it's ready

It also let us handle the annoying pieces like min scipy versions ;). We have established patterns to handle such things, but its hard for a new contributor to find them at first glance

@hollymandel hollymandel force-pushed the interp_vectorize branch 2 times, most recently from 425874c to 76defd1 Compare September 24, 2024 21:40
@dcherian
Copy link
Contributor

(fyi we squash on merge, so you needn't squash every change)

dcherian
dcherian approved these changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work! Thanks for taking this on, we've needed someone to do this for a while now :)

One last request, please add a note to whats-new.rst

@dcherian dcherian changed the title Interp vectorize Support vectorized interpolation with more scipy interpolators Sep 24, 2024
@dcherian dcherian added the plan to merge Final call for comments label Sep 24, 2024
@max-sixty max-sixty merged commit e239389 into pydata:main Sep 25, 2024
28 checks passed
@max-sixty
Copy link
Collaborator

Thanks @hollymandel !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcherian dcherian dcherian approved these changes

Assignees
No one assigned
Labels
plan to merge Final call for comments topic-interpolation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot use documented interp() methods due to "vectorizeable_only" check and kwargs name clash
4 participants
@hollymandel @max-sixty @keewis @dcherian